Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alertmanager: Avoid breaking /api/v1/alerts if legacy prefix is empty #3905

Merged
merged 1 commit into from
Mar 5, 2021
Merged

alertmanager: Avoid breaking /api/v1/alerts if legacy prefix is empty #3905

merged 1 commit into from
Mar 5, 2021

Conversation

nickbp
Copy link
Contributor

@nickbp nickbp commented Mar 3, 2021

If the legacy prefix is an empty string, it's interpreted as a handler for /*. Any handlers added afterwards are silently ignored, so requests to /api/v1/alerts are routed to the legacy handler, rather than to am.*UserConfig as would be expected. Meanwhile any handlers added BEFORE the /* legacy handler still work as expected - making this very difficult to debug!

Signed-off-by: Nick Parker nick@nickbp.com

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@pracucci pracucci requested a review from jtlisi March 4, 2021 16:17
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

nit: @nickbp I think this warrants a bugfix changelog entry? WDYT @pracucci

@pracucci
Copy link
Contributor

pracucci commented Mar 4, 2021

nit: @nickbp I think this warrants a bugfix changelog entry? WDYT @pracucci

Sure, let's add a CHANGELOG entry.

If the legacy prefix is an empty string, it's interpreted as a handler for `/*`. Any handlers added afterwards are silently ignored, so requests to `/api/v1/alerts` are routed to the legacy handler, rather than to `am.*UserConfig` as would be expected. Meanwhile any handlers added BEFORE the `/*` legacy handler still work as expected - making this very difficult to debug!

Signed-off-by: Nick Parker <nick@nickbp.com>
@nickbp
Copy link
Contributor Author

nickbp commented Mar 4, 2021

Added an entry to CHANGELOG.md

@pracucci pracucci merged commit a0c89bd into cortexproject:master Mar 5, 2021
roystchiang pushed a commit to roystchiang/cortex that referenced this pull request Apr 6, 2022
…cortexproject#3905)

If the legacy prefix is an empty string, it's interpreted as a handler for `/*`. Any handlers added afterwards are silently ignored, so requests to `/api/v1/alerts` are routed to the legacy handler, rather than to `am.*UserConfig` as would be expected. Meanwhile any handlers added BEFORE the `/*` legacy handler still work as expected - making this very difficult to debug!

Signed-off-by: Nick Parker <nick@nickbp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants